adding fields for best xhat and recent xhats #530
Conversation
bknueven
requested review from
DLWoodruff and
Copilot
and removed request for
Copilot
There was a problem hiding this comment.
Pull Request Overview
This pull request addresses issue #528 by refactoring objective value handling and extending support for tracking xhat information. Key changes include:
- Replacing the legacy _obj_from_agnostic field with inner_bound across multiple modules.
- Introducing new fields BEST_XHAT and RECENT_XHATS to better manage solution buffers.
- Refactoring components in spbase, spwindow, and spcommunicator to support the new xhat features and buffers.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| mpisppy/tests/test_component_map_usage.py | Adds tests to verify the component map usage for variable values. |
| mpisppy/spopt.py | Updates calculation of expected objective to use inner_bound. |
| mpisppy/spbase.py | Adds caching for the latest solution before updating the best solution. |
| mpisppy/extensions/xhatbase.py | Updates objective value assignment for xhat communication. |
| mpisppy/cylinders/spwindow.py | Refactors field definitions and parameter naming to include new xhat-related fields. |
| mpisppy/cylinders/spoke.py | Expands send/receive fields and xhat sending logic in spoke implementations. |
| mpisppy/cylinders/spcommunicator.py | Introduces circular buffer classes for send/receive operations. |
| Various mpisppy/agnostic/* files and farmer examples | Replaces use of _obj_from_agnostic with inner_bound consistently. |
| .github/workflows/test_pr_and_main.yml | Adds a new workflow step to execute the ComponentMap test. |
Comments suppressed due to low confidence (1)
mpisppy/cylinders/spwindow.py:45
- [nitpick] For internal consistency, consider either prefixing this user-modifiable parameter with an underscore (e.g., _total_number_recent_xhats) if it's meant for internal use, or update the accompanying documentation to clarify its intended usage.
field_length_components.total_number_recent_xhats = pyo.Param(mutable=True, initialize=10, within=pyo.NonNegativeIntegers)
Co-authored-by: Copilot <[email protected]>
DLWoodruff
left a comment
DLWoodruff
left a comment
There was a problem hiding this comment.
I have to admit that I am just taking the lower-level changes on faith. If you need a check, we should look together on Friday
The one thing I am not sold on, and I think it's worth some discussion, it keeping the "recent" xhats. It's there for FWPH (#529), but I'm not even sure it's of value there. I wouldn't mind a deeper review on this, if you're available to chat about it on Friday. |
bknueven
left a comment
bknueven
left a comment
There was a problem hiding this comment.
Feedback from live review
Correct bug introduced by #530
2 participants
Will fix #528